-
Notifications
You must be signed in to change notification settings - Fork 43
Reiterate layout step until section addresses converge #688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
81a1a0b to
7b61c10
Compare
7b61c10 to
2d6821f
Compare
2d6821f to
93a16f2
Compare
quic-seaswara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you exposing maxPreRelaxationPasses to the function ?
I feel createProgramHeaders is becoming overly iterative.
The logic can be contained in GNULDBackend.
We need to use
To push the logic in GNULDBackend, we will need to refactor |
93a16f2 to
42473f8
Compare
42473f8 to
485a38d
Compare
| while (out != outEnd) { | ||
| bool useSetLMA = false; | ||
| int preRelaxPassIter = 0; | ||
| do { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a massive loop that requires indenting everything and makes the diff really large. I wonder if this can be avoided somehow? Can we isolate the single-pass into a new function and then have a small loop in relax() or somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we isolate the single-pass into a new function and then have a small loop in relax() or somewhere else?
That will also result in a huge diff, same as it is right now.
I wonder if this can be avoided somehow?
We can keep the indentation of the original loop as before, but still enclose it within do .. while(), however, then the indentation would be incorrect and reading the code in future might be misleading in the future due to incorrect indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to first refactor the single-pass in a separate patch, and then this PR just becomes adding a loop
If refactoring is not an option, can the loop be a while() loop instead of a do...while? Since the function is massive it's not obvious what condition we are looping over until we find the end of the loop many lines down.
Regardless of whatever decision is taken here I think the two .hpp files could benefit from refactoring somewhere down the line.
| GNUVerDefFragment *GNUVerDefFrag = nullptr; | ||
| std::unordered_map<const ResolveInfo *, uint16_t> OutputVersionIDs; | ||
| #endif | ||
| OutputSectionEntry * changedOutputSection = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout iterations only continue when section addresses change, but what about symbol-only forward references?
cat > 1.c << \!
int foo() {return 0;}
!
cat > cycle.t << \!
SECTIONS {
. = 0x1000;
.text : {*(.text*)}
a = b + 4;
b = a + 4;
}
!
cat > sym.t << \!
PHDRS {TEXT PT_LOAD;}
SECTIONS {
. = 0x1000;
.text : {*(.text*)} : TEXT
a = b + 4;
b = c + 4;
c = 0x2000;
}
!
clang -c 1.c
ld.eld 1.o -T sym.t # a,b,c = 0x8, 0x2004, 0x2000
ld.lld 1.o -T sym.t # a,b,c = 0x2008, 0x2004, 0x2000
ld.lld 1.o -T cycle.t # error: assignment to symbol a does not converge
ld.eld 1.o -T cycle.t # a = 0x14; b=0x18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout iterations only continue when section addresses change, but what about symbol-only forward references?
I plan to handle symbol address convergence in the next patch. The subsequent patches will add support for symbol address convergence, improved diagnostics related to layout iterations and convergence, and some heuristics to reduce the number of iterations wherever possible.
This commit modifies the layout step to reiterate until the section addresses converge. The reiteration has an upper limit of 4. On reaching this limit, a note is emitted and the layout reiteration is stopped. It is not warning as of now because it can cause link failure when --fatal-warnings is used. This note will later be converted to a warning once the feature is complete. Please note that this reiteration limit is for layout iterations before relaxations take place. After a layout relaxation pass, the reiteration count is reset to 0. Resolves qualcomm#687 Signed-off-by: Parth Arora <partaror@qti.qualcomm.com>
485a38d to
27b20eb
Compare
| }; | ||
|
|
||
| bool linkerScriptHasMemoryCommand = m_Module.getScript().hasMemoryCommand(); | ||
| auto reset_state = [&](OutputSectionEntry *OS = nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we reuse the segment table across passes; segments from a transient layout can stick around:
In the first pass we create a new PT_LOAD for .bar due to the forward reference that remains even after v is fixed:
cat > 1.c << \!
[[gnu::section(".foo")]] int foo = 1;
[[gnu::section(".bar")]] int bar = 2;
!
cat > fwd.t << \!
SECTIONS {
. = u;
.foo : {*(.foo)}
. = v;
.bar : {*(.bar)}
u = 0x1000;
v = u + 0x10;
}
!
cat > nofwd.t << \!
SECTIONS {
. = 0x1000;
.foo : {*(.foo)}
. = 0x1010;
.bar : {*(.bar)}
}
!
clang -c 1.c
ld.eld 1.o -T fwd.t -o fwd # two PT_LOADs with .foo and .bar separate.
ld.eld 1.o -T nofwd.t -o nofwd # single PT_LOAD with .foo and .bar together.
ld.lld 1.o -T fwd.t -o fwd
ld.lld 1.o -T fwd.t -o nofwd
llvm-readelf -l fwd nofwd| #endif | ||
| OutputSectionEntry * changedOutputSection = nullptr; | ||
| const int maxPreRelaxationPasses = 4; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like lld's max passes is 5. Is there any reason we picked a different number?
| DIAG(warn_empty_segment, DiagnosticEngine::Warning, | ||
| "Empty segment: '%0'") | ||
| "Empty segment: '%0'") | ||
| DIAG(note_section_address_not_converging, DiagnosticEngine::Note, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a warning instead?
| hasError = true; | ||
| } | ||
| setVMA(*cur, vma, /*ignoreChangedSection=*/false); | ||
| cur->setAddr(vma); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need both setVMA and cur->setAddr()?
This commit modifies the layout step to reiterate until the section addresses converge. The reiteration has an upper limit of 4. On reaching this limit, a warning is emitted and the layout reiteration is stopped.
Please note that this reiteration limit is for layout iterations before relaxations take place. After a layout relaxation pass, the reiteration count is reset to 0.
Resolves #687